Add graphman unassign/reassign commands to graphman graphql api #5678
Conversation
isum
left a comment
isum
left a comment
There was a problem hiding this comment.
The code looks good, but there are a few things to consider before I can complete my review:
- I would suggest rethinking the approach with warnings, so that we are not hiding potentially important details from users. It might be a good idea to break the commands into smaller pieces to allow more flexibility in the interfaces that assemble the commands (it is important not to allow assembling wrong commands);
- When I introduced the GraphQL API, we agreed that all implemented commands should have tests, and there are no tests for the new GraphQL commands;
- The primary use case for extracting the Graphman commands into a separate crate and breaking them into smaller pieces was to share the functionality between the CLI and the GraphQL API, but this PR does not share the extracted code with the CLI;
isum
left a comment
isum
left a comment
There was a problem hiding this comment.
It looks good, but I would suggest you use the extracted functionality of reassign and unassign in the CLI modules to remove some code duplication. The goal is to eventually share as much functionality as possible between the CLI and the GraphQL API.
shiyasmohd
force-pushed
the
shiyasmohd/add-unassign-reassign-to-graphman-api
branch
2 times, most recently
from
63f0a8f to
8bdc003
Compare
…loyment mutation tests
shiyasmohd
force-pushed
the
shiyasmohd/add-unassign-reassign-to-graphman-api
branch
from
827491c to
d0d7d5c
Compare
| pub enum ReassignResult { | ||
| EmptyResponse, | ||
| CompletedWithWarnings(Vec<String>), | ||
| } |
There was a problem hiding this comment.
Just a note. It is not very common for a Rust function to return T::EmptyResponse on success (it makes sense for GraphQL types because of GraphQL style and conventions). Maybe a better name for this variant is ReassignResult::Ok, but that's not really important right now
There was a problem hiding this comment.
Got it. I've created a new issue for that.
No description provided.